Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't check participants' whole names in default rooms #4141

Merged
merged 2 commits into from
Jul 20, 2021
Merged

Don't check participants' whole names in default rooms #4141

merged 2 commits into from
Jul 20, 2021

Conversation

TomatoToaster
Copy link
Contributor

@TomatoToaster TomatoToaster commented Jul 19, 2021 •
edited
Loading

@thienlnam, please review

Details

Now for sure, we won't be showing rooms that user's are in when you're searching their name (unless they share a name with a default room).

Also fixing a Podfile.lock spec checksum that's out of date.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/169139
specifically: https://github.com/Expensify/Expensify/issues/169139#issuecomment-882776491

Tests

Do the Web QA locally.

QA Steps

Since defaultRooms are only available to expensify employees rn, I can test this. Ping @TomatoToaster, if I haven't already checked this off for QA.

  1. Search for a person's name and verify it only shows their dms and does not show rooms they are in. You'l have to type their full first name of full last name, to truly verify this works. If their name is a substring of room name, technically it should show that room name 😓 so try a name like amal or shawn.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Good:
image
Bad:
image

Web

@TomatoToaster TomatoToaster requested a review from a team as a code owner July 19, 2021 21:21
@TomatoToaster TomatoToaster self-assigned this Jul 19, 2021
@MelvinBot MelvinBot requested review from thienlnam and removed request for a team July 19, 2021 21:21
TomatoToaster
TomatoToaster commented Jul 19, 2021
View reviewed changes
ios/Podfile.lock
@@ -795,7 +795,7 @@ SPEC CHECKSUMS:
DoubleConversion: cf9b38bf0b2d048436d9a82ad2abe1404f11e7de
EXHaptics: 337c160c148baa6f0e7166249f368965906e346b
FBLazyVector: 7b423f9e248eae65987838148c36eec1dbfe0b53
FBReactNativeSpec: e564123bce1111e84dc7aa0765fb1175f0c48aa0
FBReactNativeSpec: d65967936e86fe0fe6cca5c0125c237636868d4a
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to the real problem here, but this should probably be updated according to this: https://expensify.slack.com/archives/C01GTK53T8Q/p1626728088492500

To fully confirm, can you also run pod install in the ios folder on the main branch and verify that you also get a different checksum? Also, run it on this branch, amal-participants-full-name-check, and it should give you the same checksum as me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got a different checksum for a bunch of things

RNReanimated (2.3.0-alpha.1):
DoubleConversion: cde416483dac037923206447da6e1454df403714
FBReactNativeSpec: 76cede005e0667703f69105017804cc6c5e39fc8
glog: 40a13f7840415b9a77023fbcae0f1e6f43192af3
RNReanimated: 833ebd229b31e18a8933ebd0cd744a0f47d88c42

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah hmm let me pull main into this branch and see if that resolves it. If not I might just not mess with this part.

@TomatoToaster TomatoToaster mentioned this pull request Jul 19, 2021
Merged
5 tasks
thienlnam
thienlnam reviewed Jul 19, 2021
View reviewed changes
Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good but it seems like this will remove the ability to find any default room in the search?

@TomatoToaster
Copy link
Contributor Author

@thienlnam Ah! Don't worry this is not the only code for generating search text.

You can see it more in this PR: #3847 but when we build the search matches for default rooms, we use their reportName and their policy name (or domainName if it is a domain room).

Here's where that code specifically is: https://github.com/Expensify/Expensify.cash/blob/1e1a9a21a2b238749cc3bdc77fc85f4635d1878f/src/libs/OptionsListUtils.js#L154-L177

So the isSearchStringMatch function I'm modifying in this PR will also search those values with matchRegex.test(valueToSearch)

For proof, here's me searching by policy name and room names:
image
image
image

@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2021 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@thienlnam
Copy link
Contributor

Screen Shot 2021-07-20 at 9 27 03 AM

Doing some testing and noticed that a lack of results just outputs the text under the search bar, is this expected behavior?

@thienlnam
Copy link
Contributor

Also, looks like you'll need to sign the #4141 (comment)

@TomatoToaster
Copy link
Contributor Author

TomatoToaster commented Jul 20, 2021 •
edited
Loading

Yeah I think that behavior is expected since it happens on prod too:
image

Also hmm I just reacknowledged it here since that comment: #4068 (comment)

Have you done it again since the repo name change? (I'll do it again just in case though)

@TomatoToaster
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

thienlnam
thienlnam approved these changes Jul 20, 2021
View reviewed changes
Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet, thanks!

@thienlnam thienlnam merged commit ff9b55b into main Jul 20, 2021
@thienlnam thienlnam deleted the amal-participants-full-name-check branch July 20, 2021 17:21
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

This was referenced Jul 20, 2021
Closed
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.79-5🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@isagoico
Copy link

@TomatoToaster Heyo! Following PR orders to bump you for QA :)

@TomatoToaster
Copy link
Contributor Author

This passed qa too checking it off image

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.80-2🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thienlnam thienlnam thienlnam approved these changes

Assignees

@TomatoToaster TomatoToaster

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TomatoToaster @thienlnam @OSBotify @isagoico